Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Violation Highlight Styles #28

Merged

Conversation

BrianSipple
Copy link
Member

@BrianSipple BrianSipple commented Jun 15, 2016

This PR attempts to address some of the challenges outlined in #19 -- which seems like a great use case for striped gradients. (@nathanhammond, your right on that it would be difficult to define a color that will apply to all UI styles but with striped gradients, we aren't limited to just one 😀).

I put together a Codepen demo with some of the ideas that I've been prototyping for a violation warning -- which includes a few variants of striped repeating linear gradients and SVG filters. And at the moment, I decided on this:

screen shot 2016-06-15 at 6 15 45 pm

The color scheme (transparent / mint-yellow / aqua-green / navy-blue, seen here over a black background) was chosen so that it will provide a strong contrast against either light or dark background -- with special care to ensure that this was also true for color-blind users. I also added a rule that removes the gradient on hover so that a user can always inspect the original element when they see the warning appear over it. Overall, this approach feels a lot more clean and performant than creating new markup and trying to perform reads/writes in the DOM to position it correctly over each component.

But that's just our default. Which takes effect by the audit function applying the axe-violation class to offending nodes. For customization purposes, I added logic that allows a user to have their own classes applied instead -- by exposing an axeViolationClassNames property that takes an array of user-provided class names. This array then gets used instead of our default name when the audit function adds classes:

Ember.$(node.target.join(','))[0].classList.add(...violationClassNames);

The more I think about it, this can also be one of the options we allow at the ENV config level while we're at it.

Furthermore, if someone really wants to do heavy stuff with their violating components in JavaScript, they'd already have the ability to do that through the axeCallback that we support. Perhaps we could add a few "recipes" to the README for how to do this if it seems like there's a significant use case?

hsla(166, 44%, 65%, 1.00) 1.2em,
hsla(214, 96%, 45%, 1.00) 1.2em,
hsla(214, 96%, 45%, 1.00) 1.6em
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to !important this since the specificity here is very low.

Copy link
Member Author

@BrianSipple BrianSipple Jun 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I wanted to bring this up as an open question as well.

Part of me feels like it would be good to leave off !important here. The low specificity isn't too concerning so long as no one defines their own .axe-violation rules (i.e: we're not just trying to target div or something) -- and if they do, that may well be a valid use case that we shouldn't get in the way of.

But then I can see the other side: .axe-violation is our special sauce -- and, for their own good, someone using ember-a11y-testing shouldn't try to override it, as it would only add ambiguity to their markup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more saying that another selector targeting an invalid element like #something .blue-background will win compared to ours without !important.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh.... yep, that's absolutely true. I was only considering using the same class. Thanks for thinking outside of the box model.

@nathanhammond
Copy link
Contributor

That is certainly going to get somebody's attention! I like the solution. A lot. You have a couple of failing tests that seem like they're simply relying on an element's existence which can be undefined but other than that I'd like to land this. 😄

@BrianSipple BrianSipple force-pushed the improve-violation-highlight-styles branch from 0fc31b4 to f5c91eb Compare June 16, 2016 01:12
@BrianSipple
Copy link
Member Author

I'm starting to think that a greater proportion of transparency looks better between those stripes -- and that the stripes should be moving "forward":

screen shot 2016-06-15 at 6 48 15 pm

@BrianSipple BrianSipple force-pushed the improve-violation-highlight-styles branch from 3d2983f to 7769ec2 Compare June 17, 2016 02:54
@nathanhammond
Copy link
Contributor

I like it. It's also possible that we should scale the background-image to make sure that tiny elements still get "striped."

@BrianSipple BrianSipple force-pushed the improve-violation-highlight-styles branch 2 times, most recently from bc33251 to 8729243 Compare June 21, 2016 02:59
@BrianSipple BrianSipple changed the title [WIP] Improve Violation highlight styles [WIP] Improve Violation Highlight Styles Jun 21, 2016
@BrianSipple
Copy link
Member Author

BrianSipple commented Jun 21, 2016

@nathanhammond Hmm... if I'm understanding what you're getting at by "scale", that's what I had in mind by using percentage values for the gradient stops to render the screenshot above (the percentage being relative to the container size along the gradient axis):

repeating-linear-gradient(
    135deg,
    transparent,
    transparent 25%,   /* make transparent portion somewhat wider than total stripe width */  
    var(--color--mint-yellow) 25%,
    var(--color--mint-yellow) 30%,
    var(--color--aqua-green) 30%,
    var(--color--aqua-green) 35%,
    var(--color--navy-blue) 35%,
    var(--color--navy-blue) 40%
  );

* Variables here are from the Codepen, but raw values are all we'd actually use.

More to that point, though, I'm also leaning towards if ems (font-size of the current container) as a better hook for relative sizing. That way, if you had a large container -- perhaps even with a transparent background -- with just a small amount text as the visible content, the text would still get striped. Below is the "card" with em gradient stop sizing, and then two text examples...

screen shot 2016-06-21 at 2 30 30 am

Percentage-based gradient stop sizing:

screen shot 2016-06-21 at 2 31 15 am

em-based gradient stop sizing:

screen shot 2016-06-21 at 2 31 34 am

... Kind of digging the ems 😄.

@BrianSipple BrianSipple force-pushed the improve-violation-highlight-styles branch 2 times, most recently from 5db2727 to 2ee1e60 Compare June 21, 2016 09:58
import Ember from 'ember';

export default Ember.Route.extend({
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can delete this file.

@nathanhammond
Copy link
Contributor

@BrianSipple Turns out I didn't understand your code and you were already way ahead of me on this. I like the way the smaller sizes work out. At this point I'm deferring to your implementation. I have no additional comments on the code. So, :shipit:.

axeCallback ... "recipes" to the README for how to do this if it seems like there's a significant use case

Let's keep this in our back pocket and watch for feature requests as they come in that can be solved with this.

@BrianSipple BrianSipple force-pushed the improve-violation-highlight-styles branch from c3ed436 to 8ecfe65 Compare June 23, 2016 07:07
@BrianSipple BrianSipple changed the title [WIP] Improve Violation Highlight Styles Improve Violation Highlight Styles Jun 23, 2016
@BrianSipple BrianSipple merged commit ff2e794 into ember-a11y:master Jun 23, 2016
@BrianSipple BrianSipple deleted the improve-violation-highlight-styles branch June 23, 2016 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants